Implement row group skipping for the default engine parquet readers by scovich · Pull Request #362 · delta-io/delta-kernel-rs

scovich · 2024-09-27T20:53:00Z

Previous PR #357 implemented the logic of stats-based skipping for a parquet reader, but in abstract form that doesn't actually depend on parquet footers. With that in place, we can now wire up the kernel default parquet readers to use row group skipping.

Also fixes #380.

codecov · 2024-09-27T20:57:13Z

Codecov Report

Attention: Patch coverage is 85.61525% with 83 lines in your changes missing coverage. Please review.

Project coverage is 77.66%. Comparing base (1c4b9ce) to head (4a77f3a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/parquet_row_group_skipping.rs	52.79%	26 Missing and 50 partials ⚠️
kernel/src/engine/default/parquet.rs	50.00%	5 Missing ⚠️
kernel/src/snapshot.rs	88.88%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   77.06%   77.66%   +0.59%     
==========================================
  Files          47       49       +2     
  Lines        9524    10079     +555     
  Branches     9524    10079     +555     
==========================================
+ Hits         7340     7828     +488     
- Misses       1790     1805      +15     
- Partials      394      446      +52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich · 2024-09-27T22:29:55Z

kernel/src/scan/mod.rs

            engine,
            commit_read_schema,
            checkpoint_read_schema,
-            self.predicate.clone(),


NOTE: This was an existing bug -- passing a query-level filter to the metadata file reads.

(We probably should add a meta-skipping unit test for replay?)

opened #380

Already done -- #381 added three new replay_for_XXX tests, which this PR updates to account for the expected row group skipping.

ah sorry! thanks!!

scovich · 2024-09-27T22:32:14Z

kernel/src/snapshot.rs

            .map_ok(|batch| (batch, true));

        let parquet_client = engine.get_parquet_handler();
-        // TODO change predicate to: predicate AND add.path not null


I removed these two TODO because the P&M query also invokes this code path, and filtering by add.path NOT NULL is just plain incorrect. Also, see the code comment at the other replay call site for why it doesn't make sense to pass add.path IS NOT NULL as a row group skipping filter.

kernel/tests/read.rs

kernel/src/engine/parquet_row_group_skipping.rs

kernel/tests/read.rs

kernel/src/engine/parquet_row_group_skipping.rs

kernel/tests/read.rs

kernel/src/engine/default/parquet.rs

This reverts commit 6236874.

kernel/src/engine/parquet_row_group_skipping.rs

nicklan

Looks great, thanks. Just a couple of small nits

ffi/src/engine_funcs.rs

kernel/src/engine/parquet_row_group_skipping.rs

kernel/tests/read.rs

scovich · 2024-10-08T18:16:17Z

kernel/src/engine/parquet_row_group_skipping.rs

-        Some(self.get_stats(col)?.null_count_opt()? as i64)
+        let nullcount = match self.get_stats(col) {
+            Some(s) => s.null_count_opt()? as i64,
+            None => self.get_rowcount_stat_value(),


This is a new find, exposed on accident by me hacking two more parts into the checkpoint so we could test transaction app id filtering (the "checkpoint" schema was truncated, which prevented the P&M query from skipping those parts)

If the statistics() method on ColumnChunkMetadata returns None, that just means that there are no stats for that column, but doesn't necessarily imply that all values are null does it?

Oh, good catch. I didn't put the check deep enough. There are three levels of None here:

The column chunk doesn't even exist (infer nullcount = rowcount)

The column chunk doesn't have stats (should not infer anything clever)

The stats object lacks a particular stat

To make things even more "fun", we have the following warning in Statistics::null_count_opt 🤦:

this API returns Some(0) even if the null count was not present in the statistics

So I have two problems to work around now.

Both fixed.

kernel/src/expressions/mod.rs

kernel/src/scan/mod.rs

kernel/src/snapshot.rs

scovich · 2024-10-08T18:20:40Z

kernel/src/snapshot.rs

+    // txn.appId    BINARY  0      "3ae45b72-24e1-865a-a211-3..." / "3ae45b72-24e1-865a-a211-3..."
+    // txn.version  INT64   0      "4390" / "4390"
+    #[test]
+    fn test_replay_for_metadata() {


An accidentally clever test :P
(see other comment)

scovich · 2024-10-08T18:33:18Z

kernel/src/transaction.rs

+        // checkpoint part when patitioned by `add.path` like the Delta spec requires. There's no
+        // point filtering by a particular app id, even if we have one, because people usually query
+        // for app ids that exist.
+        let meta_predicate = Expr::column("txn.appId").is_not_null();


The code didn't previously attempt row group skipping for app ids. Now it does.

zachschuermann

given the size of this PR i'll say this is a best-effort review but LGTM

zachschuermann · 2024-10-08T20:50:20Z

kernel/src/scan/mod.rs

            engine,
            commit_read_schema,
            checkpoint_read_schema,
-            self.predicate.clone(),


opened #380

nicklan

Had a question about correctness, but otherwise lgtm

nicklan · 2024-10-08T21:52:16Z

kernel/src/engine/parquet_row_group_skipping.rs

-        Some(self.get_stats(col)?.null_count_opt()? as i64)
+        let nullcount = match self.get_stats(col) {
+            Some(s) => s.null_count_opt()? as i64,
+            None => self.get_rowcount_stat_value(),


If the statistics() method on ColumnChunkMetadata returns None, that just means that there are no stats for that column, but doesn't necessarily imply that all values are null does it?

nicklan · 2024-10-08T21:54:31Z

kernel/src/snapshot.rs

+    // txn.appId    BINARY  0      "3ae45b72-24e1-865a-a211-3..." / "3ae45b72-24e1-865a-a211-3..."
+    // txn.version  INT64   0      "4390" / "4390"
+    #[test]
+    fn test_replay_for_metadata() {


In preparation for #362 that actually implements parquet row group skipping, here we make various preparatory changes that can stand on their own: * Plumb the predicates through to the parquet readers, so that they can easily start using them * Add and use a new `Expression::is_not_null` helper that does what it says * Factor out `replay_for_XXX` methods, so that log replay involving push-down predicates can be tested independently. * Don't involve <n>.json in log replay if <n>.checkpoint.parquet is available This should make both changes easier to review.

nicklan

lgtm! two small suggestions that you can take or leave as you prefer.

nicklan · 2024-10-09T22:05:52Z

kernel/src/engine/parquet_row_group_skipping.rs

+        !matches!(result, Some(false))
+    }
+
+    /// Returns `None` if the column doesn't exist and `Some(None)` if the column has no stats.


we could define an enum for this rather than Some(None). Just a thought, I'm okay with both ways, and the Some(None) will have cleaner code in the method (at the cost of a more confusing return type)

oh nice i think i commented on this too - maybe just Result? and we can have Err(MissingColumn) for a more understandable return type?

I avoided Result because that would be exceptions as control flow (this isn't actually an error, it's just a situation).

If this were public code, the enum might make sense. But for a private method, I don't think it's worth the cognitive overhead (both to define and use it) when one line of code comment can fully explain what's going on?

Yeah, I think it's fine as is since both the method and the call site have a comment.

kernel/src/engine/parquet_row_group_skipping.rs

zachschuermann

another stamp with a few more questions :)

kernel/src/engine/parquet_row_group_skipping.rs

kernel/src/scan/mod.rs

kernel/src/engine/parquet_row_group_skipping/tests.rs

zachschuermann · 2024-10-09T22:12:29Z

kernel/src/engine/parquet_row_group_skipping/tests.rs

do we need to introduce testing for the E2E stats to skipping path? this test suite does all of the stats part it seems - are we relying on existing tests to make sure the remaining skipping based on these stats is correct?

I think that's what the test in read.rs was doing before I deleted it... should I reinstate?

do we think E2E path is covered without it? if not then I'm not opposed to one more test :)

Actually, I think test_data_row_group_skipping should cover it pretty well? It plumbs the predicate through starting from the scan builder. The only remaining coverage would be FFI and the toy table reader -- neither of which has any predicates that they could pass. If/when those get updated to support predicates at all, they should be able to test that the predicates actually work.

ryan-johnson-databricks and others added 11 commits September 25, 2024 08:42

WIP - first pass at the code

715f233

split out a trait, add more type support

ef71f1a

support short circuit junction eval

39b8927

Merge remote-tracking branch 'oss/main' into row-group-skipping

b5c3a52

add tests, fix bugs

e71571e

support SQL WHERE semantics, finished adding tests for skipping logic

cbca3b3

Mark block text as not rust code doctest should run

e7d87eb

add missing tests identified by codecov

beeb6e8

Wire up row group skipping

519acbd

delete for split - parquet reader uses row group skipping

18b33cf

parquet reader now uses row group skipping

6c98441

scovich requested review from OussamaSaoudi-db, hntd187 and nicklan September 27, 2024 20:53

OussamaSaoudi-db mentioned this pull request Sep 27, 2024

Add visitor for converting kernel expressions to engine expressions #363

Merged

scovich mentioned this pull request Sep 27, 2024

Utility trait for stats-based skipping logic #357

Merged

scovich commented Sep 27, 2024

View reviewed changes

kernel/tests/read.rs Outdated Show resolved Hide resolved

scovich mentioned this pull request Sep 28, 2024

Paquet and JSON readers use Arc<Expression> to avoid deep copies #364

Merged

nicklan reviewed Oct 1, 2024

View reviewed changes

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

kernel/tests/read.rs Outdated Show resolved Hide resolved

scovich added 3 commits October 2, 2024 17:12

add stats-getter test; review comments

0fdaf0a

Merge remote-tracking branch 'oss/main' into use-row-group-skipping

8ac33f8

improve test coverage; clippy

1cf03dc

scovich commented Oct 3, 2024

View reviewed changes

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

yet more test coverage

bc8b344

OussamaSaoudi-db reviewed Oct 3, 2024

View reviewed changes

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

scovich added 2 commits October 4, 2024 08:58

improve test coverage even more

0971002

Add a query level test as well

375a380

scovich commented Oct 4, 2024

View reviewed changes

kernel/tests/read.rs Show resolved Hide resolved

scovich commented Oct 7, 2024

View reviewed changes

kernel/src/engine/default/parquet.rs Outdated Show resolved Hide resolved

scovich added 3 commits October 7, 2024 07:14

remove spurious TODO

46d19e3

Revert "Fix broken sync json parsing and harmonize file reading"

7666512

This reverts commit 6236874.

Merge remote-tracking branch 'oss/main' into use-row-group-skipping

f3865d0

scovich removed the merge hold Don't allow the PR to merge label Oct 7, 2024

OussamaSaoudi-db reviewed Oct 7, 2024

View reviewed changes

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

nicklan approved these changes Oct 8, 2024

View reviewed changes

ffi/src/engine_funcs.rs Show resolved Hide resolved

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

kernel/src/engine/parquet_row_group_skipping.rs Outdated Show resolved Hide resolved

kernel/tests/read.rs Outdated Show resolved Hide resolved

review comments

a4dc3da

scovich requested a review from OussamaSaoudi-db October 8, 2024 02:52

scovich added 5 commits October 8, 2024 10:08

Merge remote-tracking branch 'oss/main' into use-row-group-skipping

40131db

Infer null count stat for missing columns; add more tests

bf65904

One last test

cce762d

test cleanup

c7d6bb0

code comment tweak

4f92ed7

scovich commented Oct 8, 2024

View reviewed changes

zachschuermann approved these changes Oct 8, 2024

View reviewed changes

kernel/src/scan/mod.rs

engine,

commit_read_schema,

checkpoint_read_schema,

self.predicate.clone(),

Copy link

Collaborator

zachschuermann Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #380

scovich mentioned this pull request Oct 8, 2024

Prepare to support parquet row group skipping #381

Merged

nicklan reviewed Oct 8, 2024

View reviewed changes

remove unneeded test

08a305b

scovich added 2 commits October 8, 2024 20:30

Merge remote-tracking branch 'oss' into use-row-group-skipping

e8a947e

fix two nullcount stat bugs

bf1e3a8

scovich requested a review from nicklan October 9, 2024 03:37

nicklan approved these changes Oct 9, 2024

View reviewed changes

zachschuermann approved these changes Oct 9, 2024

View reviewed changes

scovich added 2 commits October 9, 2024 15:54

Merge remote-tracking branch 'oss/main' into use-row-group-skipping

9d632e7

review nits

4a77f3a

scovich merged commit 42afc06 into delta-io:main Oct 9, 2024

scovich deleted the use-row-group-skipping branch November 8, 2024 21:00

Conversation

scovich commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

scovich Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

scovich commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

scovich Sep 27, 2024 •

edited

Loading

scovich Oct 9, 2024 •

edited

Loading

scovich Sep 27, 2024 •

edited

Loading